Drop go.uber.org/atomic in favor of sync/atomic #3443
Conversation
The primitives in sync/atomic are not as good as those in uber/atomic; happy to drop this.
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
| // onceError is an object that will only store an error once. | ||
| type onceError struct { | ||
| err atomic.Value | ||
| } | ||
|
|
||
| // Store stores the err. | ||
| func (e *onceError) Store(err error) { | ||
| e.err.CompareAndSwap(nil, err) | ||
| } | ||
|
|
||
| // Load loads the stored error. | ||
| func (e *onceError) Load() error { | ||
| err, ok := e.err.Load().(error) | ||
| if !ok { | ||
| return nil | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
took this from a bufstream PR; could use a raw atomic.Value inline, but this prevents needing to do the type assertion at the callsite. 🤷.
Oops!
| func (e *onceError) Load() error { | ||
| err, ok := e.err.Load().(error) | ||
| if !ok { | ||
| return nil |
There was a problem hiding this comment.
This seems like a really bad situation - I need to review my Golang a bit, but if this is non-nil, this means that the thing that was stored was not an error on e.err, which would be a system error in itself.
There was a problem hiding this comment.
yeah, I think for safety purposes you could move onceError to a different package so someone within this package can't muck with the e.err value directly (instead of using Store)?
There was a problem hiding this comment.
Maybe, but that's not really what I'm saying - I'm saying that you are returning nil when ok == false, and ok should only be false if e.err.Load() is not of type error, which would be a really bad scenario (somehow something other than an error got stored on e.err. ok might also be falseife.err.Load()isnil`, but this is where my Golang is failing me - I assume that's the only thing that is meant to be captured here.
I would imagine that if you want to do something like this, you need to do:
errIface := e.err.Load()
if errIface == nil {
return nil
}
err, ok := errIface.(error)
if !ok {
return syserror.Newf("expected error but got %T", errIface)
}
return errThere was a problem hiding this comment.
yeah, that looks more technically correct to me - if you somehow stored something that isn't type error, you'd always just get nil currently. I'll update to your approach.
There was a problem hiding this comment.
1a41b94 - feel free to change, just preferred the atomicValue var name.
There was a problem hiding this comment.
lgtm - I would probably update bufstream as well
The primitives in sync/atomic are not as good as those in uber/atomic; happy to drop this.